Add Certificate Authority functionality for AD #209

krishnavema · 2025-10-06T17:42:49Z

Implement certificate authority for AD

spoore1

This is a great start and as I mentioned earlier my main initial concern is around the request()/request_smartcard() methods.

My main thought here is to make request() align more closely with what you wrote for the IPA one so we can abstract it out to the GenericProvider later. I think the current request() could be made request_enrollment() and request_smartcard() renamed to request with some minor changes.

You might also consider a method to generate the INF file based on some basic input like template, subject, keysize. Then use template to select which set of configs to use for the INF based on that.

sssd_test_framework/roles/ad.py

danlavu

Overall, this looks great, with a few minor nitpicks and a couple of larger requested changes.

sssd_test_framework/roles/ad.py

danlavu · 2025-11-10T20:56:38Z

@krishnavema I'm sorry, I did review this before I left for PTO but I didn't click submit review.

spoore1

Another glance and it's looking very good. Just a question about the PSIni module calls you have in the request methods. I can't seem to find those.

spoore1 · 2025-11-20T21:42:55Z

sssd_test_framework/roles/ad.py

+        self.host.conn.run(
+            f"""
+            $iniPath = "{inf_path}"
+            New-PsIniFile -Path $iniPath


I can't find these cmdlets on my AD server and they don't seem to be a part of PSIni from what I can tell. Maybe I'm missing something. Is this from a custom or external module?

PsIni is loaded into the image, but it is a third-party module.

sssd-ci-containers/src/ansible/roles/ad/tasks/main.yml

- name: Install powershell modules win_shell: | [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 Get-PackageProvider NuGet -ForceBootstrap Set-PSRepository -Name 'PSGallery' -InstallationPolicy Trusted Install-Module PSIni -RequiredVersion 3.1.4 -Confirm:$False

We are using it with the GPO stuff.

I think that the problem with module i guess so i reverted to normal powershell script .

danlavu

This is great, nitpicks and missing unit tests for the misc functions.

sssd_test_framework/roles/ad.py

danlavu · 2025-11-21T03:18:11Z

sssd_test_framework/roles/ad.py

+        self.host.conn.run(
+            f"""
+            $iniPath = "{inf_path}"
+            New-PsIniFile -Path $iniPath


PsIni is loaded into the image, but it is a third-party module.

sssd-ci-containers/src/ansible/roles/ad/tasks/main.yml

- name: Install powershell modules win_shell: | [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 Get-PackageProvider NuGet -ForceBootstrap Set-PSRepository -Name 'PSGallery' -InstallationPolicy Trusted Install-Module PSIni -RequiredVersion 3.1.4 -Confirm:$False

We are using it with the GPO stuff.

sssd_test_framework/roles/ad.py

sssd_test_framework/misc/__init__.py

danlavu

I haven't tested it, but besides the single nitpick, I think this looks great. It does need to be tested; tentative approval until Scott or I can test it.

sssd_test_framework/roles/ad.py

danlavu

Tentative approval, will finish the approval after I test it.

spoore1

Some notes after trying to test.

I believe the __request_enrollment() method in AD may not be necessary.

Most (if not every) smart card certificate case we'll need to cover will require an Enrollment Agent certificate because we're using Administrator to request certificates for other users. In this case, we could just create a default enrollment agent certificate for Administrator during the AD environment setup (still TBD). If we do this, we can simply have a method to get the hash for it that can be used inside of the request() method instead of passing it in to that method.

If you want to keep the code to generate the enrollment agent certificate, I would suggest making __request_enrollment() called only once during class init() or __setup() since we only need one enrollment certificate to issue other certificates.

I think request() will be used far more than request_basic() so the latter may not be necessary either.

Since request() is what should be in the generic class, and we don't have a request_basic() for IPA, I don't think we'll need this (at least for smart card testing). If you want to keep it around for other potential usage later, I'm ok with that. It should be noted though that currently running requst_basic() even with a user name specified for the subject will return a certificate with subject = CN=Administrator. That's why we have to use the "Enroll On Behalf Of" functionality to get "CN=username" for the issued certificate's subject.

spoore1 · 2026-01-13T18:51:05Z

sssd_test_framework/roles/ad.py

+        result = self.host.conn.run(
+            """
+$ca = Get-ChildItem -Path Cert:\\LocalMachine\\Root `
+    | Where-Object {$_.Subject -like '*CA*' -and $_.Issuer -eq $_.Subject} | Select-Object -First 1


I believe -like '*CA*' should be -like '*CN=ad-RootCA*' to better match the CA root certificate in our test environments. In my testing, this returned a different certificate which causes a failure when verifying the certificates on the smart card.

spoore1 · 2026-01-13T19:04:03Z

sssd_test_framework/roles/ad.py

+        :rtype: tuple[str, str, str]
+        :raises RuntimeError: If certificate request fails.
+        """
+        return self.__request_enrollment(template, subject, password, key_size)


I'm not sure I understand why request_basic() would call __request_enrollment(). I thought the latter was just for requesting "Enrollment Agent" certificates to be used to request a smart card certificate using "Enrollment On Behalf Of" functionality since we're using Administrator to request certificates.

spoore1 · 2026-01-13T19:07:49Z

sssd_test_framework/roles/ad.py

+
+    def __request_enrollment(
+        self,
+        template: str,


I would have template define a default value (i.e. "EnrollmentAgent2") so that it is not necessary to pass it in order to get an enrollment certificate.

spoore1 · 2026-01-13T19:10:12Z

sssd_test_framework/roles/ad.py

+RequestType = PKCS10
+KeyUsage = 0xA0


While this RequestType and KeyUsage are appropriate for normal certificate requests, I'm not 100% sure this will work for Enrollment Certificates. In my testing/research, it looked like it needed:

[NewRequest] Subject="CN=Administrator,CN=Users,DC=ad,DC=test" MachineKeySet=FALSE Exportable=TRUE RequestType=CMC FriendlyName="Enrollment Agent" [RequestAttributes] CertificateTemplate="EnrollmentAgent2"

spoore1 · 2026-01-13T19:11:25Z

sssd_test_framework/roles/ad.py

+            f"""
+            $infContent = @"
+[NewRequest]
+Subject = \"{subject}\"


For an Enrollment Agent certificate, I believe the subject should be CN=Administrator,CN=Users,DC=ad,DC=test. You can probably get this data from the provider config data. I just don't know the path offhand to list here.

spoore1 · 2026-01-13T19:19:32Z

sssd_test_framework/roles/ad.py

+[NewRequest]
+Subject = \"{subject}\"
+KeySpec = 1
+KeyUsage = 0xA0
+Exportable = TRUE
+RequestType = CMC
+RequesterName = \"{requester_name}\"
+ProviderName = \"Microsoft Software Key Storage Provider\"
+
+[RequestAttributes]
+CertificateTemplate = \"{template}\"
+"@


Can these lines not be indented? I thought the conn.run() would handle indentation properly as long as it was indented along with the $infContent first line.

spoore1 · 2026-01-13T19:22:07Z

sssd_test_framework/roles/ad.py

+        self,
+        template: str,
+        subject: str,
+        enrollment_agent_cert_hash: str,


I would say that the request() method should match the one in IPA so we can make the generic method function the same way. I believe the enrollment_agent_cert_hash could be looked up inside the method (either by a lookup or by creating a new one and using that hash).

spoore1 requested changes Oct 13, 2025

View reviewed changes

krishnavema force-pushed the ad-certificate-management branch 2 times, most recently from eedd166 to bec5310 Compare October 18, 2025 15:08

krishnavema requested a review from danlavu October 18, 2025 15:08

krishnavema force-pushed the ad-certificate-management branch 2 times, most recently from fc5e3ee to e3824a9 Compare November 4, 2025 02:22

krishnavema requested a review from spoore1 November 4, 2025 05:38

danlavu requested changes Nov 10, 2025

View reviewed changes

krishnavema force-pushed the ad-certificate-management branch from e3824a9 to cc761a8 Compare November 20, 2025 17:19

krishnavema requested a review from danlavu November 20, 2025 17:28

spoore1 reviewed Nov 20, 2025

View reviewed changes

danlavu reviewed Nov 21, 2025

View reviewed changes

krishnavema force-pushed the ad-certificate-management branch from cc761a8 to c55f41c Compare November 24, 2025 02:36

krishnavema requested review from danlavu and spoore1 November 24, 2025 16:38

danlavu reviewed Nov 24, 2025

View reviewed changes

sssd_test_framework/roles/ad.py Show resolved Hide resolved

Add Certificate Authority functionality for AD

01cd51d

krishnavema force-pushed the ad-certificate-management branch from c55f41c to 01cd51d Compare January 9, 2026 18:10

krishnavema requested a review from danlavu January 9, 2026 18:18

danlavu reviewed Jan 13, 2026

View reviewed changes

spoore1 requested changes Jan 13, 2026

View reviewed changes

Add Certificate Authority functionality for AD #209

Are you sure you want to change the base?

Add Certificate Authority functionality for AD #209

Uh oh!

Conversation

krishnavema commented Oct 6, 2025

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danlavu commented Nov 10, 2025

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danlavu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danlavu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danlavu left a comment •

edited

Loading